Skip to content

feat(table): v2 table bubble#772

Open
bashbunni wants to merge 27 commits intov2-expfrom
v2-table
Open

feat(table): v2 table bubble#772
bashbunni wants to merge 27 commits intov2-expfrom
v2-table

Conversation

@bashbunni
Copy link
Contributor

@bashbunni bashbunni commented Apr 4, 2025 •
edited by andreynering
Loading

uses Lip Gloss table to render the table. Also allows users to create bubbles tables from existing Lip Gloss tables.

image

Bash notes

github.com/charmbracelet/bubbles/table

incompatible changes

removed: FromValues -> seems unnecessary

Allow chaining setters

New -> changed from func(...Option) Model to func(...Option) *Model
(*Model).SetCursor: changed from func(int) to func(int) *Model
(*Model).SetHeight: changed from func(int) to func(int) *Model
(*Model).SetRows: changed from func([]Row) to func(...[]string) *Model
(*Model).SetStyles: changed from func(Styles) to func(Styles) *Model
(*Model).SetWidth: changed from func(int) to func(int) *Model

Height and Width calculations now delegated to Lip Gloss

removed: Model.Width
removed: Model.Height

Removed wrapper types Column and Row

Row

removed: Row -> using [][]string instead
refactor: WithRows -> changed from func([]Row) Option to func(...[]string) Option
refactor: Model.Rows -> changed from func() []Row to func() [][]string
refactor: Model.SelectedRow -> changed from func() Row to func() []string

Columns, now headers

refactor: WithColumns() -> WithHeaders()
refactor: Model.Columns -> Model.Headers
refactor: (*Model).SetColumns -> (*Model).SetHeaders
removed: Column

Removed viewport

removed: (*Model).UpdateViewport

compatible changes

Support Lip Gloss Border customizations

(*Model).Border: added
(*Model).BorderBottom: added
(*Model).BorderColumn: added
(*Model).BorderHeader: added
(*Model).BorderLeft: added
(*Model).BorderRight: added
(*Model).BorderRow: added
(*Model).BorderStyle: added
(*Model).BorderTop: added
(*Model).SetBorder: added

Manage styles

(*Model).OverwriteStyles: added
(*Model).OverwriteStylesFromLipgloss: added
(*Model).SetStyleFunc: added
(*Model).SetYOffset: added
NewFromTemplate: added
WithStyleFunc: added

Andrey's TODO:

  • Make selected row index and offset work independently and correctly
  • Ensure that pageup/pagedown/home/end work correctly

@bashbunni bashbunni requested a review from aymanbagabas as a code owner April 4, 2025 05:30
andreynering
andreynering reviewed Apr 4, 2025
View reviewed changes
table/table.go Outdated
Comment on lines 132 to 135
// We can't get the rows and headers from the table, so the user needs to
// provide them as arguments.
m.rows = rows
m.headers = headers
Copy link
Member

@andreynering andreynering Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I wrote a helper function on lipgloss to convert the internal Data into a [][]string:

https://github.com/charmbracelet/lipgloss/blob/c6b946e574dddb956f8642c9f84fa0219b777ed3/table/rows.go#L116

To avoid having to ask for the data again here, I think we should have a GetHeaders() and GetData() functions on lipgloss.

There is a small decision to make here. I'm not sure if GetData() should return a [][]string directly. For me, it makes more sense to make it return the internal table.Data interface and expose dataToMatrix as a public function.

Copy link
Contributor Author

@bashbunni bashbunni Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I think it makes more sense to only store the data once instead of managing the same set of values in two places (Model and lipgloss Table).

I vote that we add GetData() that returns a Data then the user can call DataToMatrix. Performance-wise it would be the same as we would probably be doing that type conversion under the hood if we were returning [][]string. This might lead users to want to include non-string values in the table... Is that something we want to support? I guess that feature could sit in the backlog for a bit while we think about it.

andreynering
andreynering reviewed Apr 4, 2025
View reviewed changes
andreynering
andreynering reviewed Apr 4, 2025
View reviewed changes
@andreynering
Copy link
Member

andreynering commented Apr 4, 2025

It seems to be working well in my tests 👏

Let me know if you have specific scenarios that you want help testing.

@bashbunni bashbunni mentioned this pull request Apr 4, 2025
Open
@andreynering andreynering mentioned this pull request May 16, 2025
Merged
@bashbunni
Copy link
Contributor Author

bashbunni commented May 19, 2025

We still need to get the number of rows and/or border heights + if it has headers to be able to jump around the content with Home/End keys and PageUp/PageDown keys. @andreynering

@andreynering andreynering requested a review from meowgorithm as a code owner May 21, 2025 17:40
andreynering
andreynering reviewed May 21, 2025
View reviewed changes
table/table.go Outdated
Comment on lines 432 to 433
// TODO make this not hard coded?
height := lipgloss.Height(table) - 6
Copy link
Member

@andreynering andreynering May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: take a look at this

@andreynering andreynering mentioned this pull request May 22, 2025
Merged
@lrstanley
Copy link
Contributor

lrstanley commented Jul 20, 2025

What's the progress on this? Tested it out briefly, and a bit of feedback so far:

  1. New() returning a pointer vs Update() not returning a border is a little cumbersome to work with.
  2. some methods return *table.Model, like SetBorder(), which is a bit confusing, as nothing else really seems to have a builder pattern.
  3. updating the data at a high frequency does seem to have some performance impact. e.g. if I try to truncate the row data on my side when the height/width changes size, the app almost freezes, using 500%+ cpu.
  4. there is no clean way to setup non-wrapping truncation. I can update the style to be inline, which technically does it, but doesn't do any kind of ellipsis, and makes a consecutive string of characters bridging two columns, which isn't very clean.
  5. text wrapping when the screen is small also causes scrolling to break. the selected element, and the viewport don't appropriately scroll when there is an entry that is overflowing. see below video.

https://cdn.liam.sh/share/2025/07/explorer_80HMdm4hK2.mp4

there also seems to be a panic under certain scenarios (note, trace shows my fork, where all I did was merge v2-table onto current v2-exp branch, and create a tag, nothing custom):

Caught panic:

runtime error: index out of range [-1]

Restoring terminal...

goroutine 1 [running]:
runtime/debug.Stack()
        /usr/local/go/src/runtime/debug/stack.go:26 +0x5e
github.com/charmbracelet/bubbletea/v2.(*Program).recoverFromPanic(0xc0001a0a00, {0xabbc60, 0xc0001813f8})
        /home/liam/go/pkg/mod/github.com/charmbracelet/bubbletea/v2@v2.0.0-beta.4/tea.go:1139 +0x165
github.com/charmbracelet/bubbletea/v2.(*Program).Run.func2()
        /home/liam/go/pkg/mod/github.com/charmbracelet/bubbletea/v2@v2.0.0-beta.4/tea.go:872 +0xe8
panic({0xabbc60?, 0xc0001813f8?})
        /usr/local/go/src/runtime/panic.go:792 +0x132
github.com/charmbracelet/lipgloss/v2/table.(*StringData).At(0xc0007100c8?, 0xaff597?, 0x0?)
        /home/liam/go/pkg/mod/github.com/lrstanley/lipgloss/v2@v2.0.0-beta.4/table/rows.go:41 +0x5d
github.com/charmbracelet/lipgloss/v2/table.(*Table).constructRow(0xc0002a8008, 0xffffffffffffffff)
        /home/liam/go/pkg/mod/github.com/lrstanley/lipgloss/v2@v2.0.0-beta.4/table/table.go:498 +0x391
github.com/charmbracelet/lipgloss/v2/table.(*Table).String(0xc0002a8008)
        /home/liam/go/pkg/mod/github.com/lrstanley/lipgloss/v2@v2.0.0-beta.4/table/table.go:352 +0x7ce
github.com/charmbracelet/bubbles/v2/table.Model.View({{{{0xc0001e8c20, 0x2, 0x2}, {{...}, {...}}, 0x0}, {{0xc0001e8c40, 0x2, 0x2}, {{...}, ...}, ...}, ...}, ...})
        /home/liam/go/pkg/mod/github.com/lrstanley/bubbles/v2@v2.0.0-beta.3/table/table.go:472 +0xdf
[...]

@lrstanley lrstanley mentioned this pull request Sep 3, 2025
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreynering andreynering andreynering left review comments

@aymanbagabas aymanbagabas aymanbagabas approved these changes

@meowgorithm meowgorithm Awaiting requested review from meowgorithm meowgorithm is a code owner

Assignees

@andreynering andreynering

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bashbunni @andreynering @lrstanley @aymanbagabas